Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Re-export macros #170

Merged
merged 6 commits into from
Jul 17, 2021
Merged

Re-export macros #170

merged 6 commits into from
Jul 17, 2021

Conversation

billy1624
Copy link
Contributor

Motivation

Our team is developing a library which requires downstream end-user to derive strum_macro::EnumIter for their custom enum.

pub trait RelationTrait: Iterable + Debug + 'static { ... }

// Derive EnumIter
#[derive(Copy, Clone, Debug, EnumIter)]
pub enum Relation {
    Cake,
    Filling,
}

However, the expanded codes from strum_macro::EnumIter looks something like this and failed to compile...
This signature, ::strum::IntoEnumIterator, implies our downstream end-user must add the strum dependency on their own.

impl ::strum::IntoEnumIterator for Relation { ... }
error[E0433]: failed to resolve: could not find `strum` in the list of imported crates
   |
39 | #[derive(Copy, Clone, Debug, EnumIter)]
   |                              ^^^^^^^^ could not find `strum` in the list of imported crates
   |

Hence, we want to find a way to re-export strum & strum_macros in our library. And downstream end-user don't need to specify the strum dependency on their own.

Proposed Solution

Adding an optional derive attribute #[strum(crate_path = "some_crate::strum")], for example...

// Derive EnumIter with crate_path derive attribute
#[derive(Copy, Clone, Debug, EnumIter)]
#[strum(crate_path = "some_crate::strum")]
pub enum Relation {
    Cake,
    Filling,
}

// Derive macro will be expanded into
impl some_crate::strum::IntoEnumIterator for Relation { ... }

Backward compatibility, if the derive attribute was not provided...

// Derive EnumIter without crate_path derive attribute
#[derive(Copy, Clone, Debug, EnumIter)]
pub enum Relation {
    Cake,
    Filling,
}

// Derive macro will be expanded into
impl ::strum::IntoEnumIterator for Relation { ... }

References

Similar feature was discussed on serde-rs/serde#1465. And serde provides a optional derive attribute to achieve it.

#[derive(Copy, Clone, Debug, Deserialize, Serialize)]
#[serde(crate = "some_crate::serde")]
struct Vertex {
    position: [u32; 3],
}

@billy1624 billy1624 mentioned this pull request Jun 30, 2021
Merged
Peternator7
Peternator7 requested changes Jul 5, 2021
View reviewed changes
Copy link
Owner

@Peternator7 Peternator7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes a lot of sense to me! Thanks for putting together a PR :) I left a few comments and if you could add a few tests, that would be great.

Maybe just try re-exporting the strum derives from some nested module in the tests and make sure it still works. You could also use the debugging section to make sure the proper code is generated.

strum_macros/src/helpers/metadata.rs Outdated
@@ -14,6 +15,7 @@ pub mod kw {

// enum metadata
custom_keyword!(serialize_all);
custom_keyword!(crate_path);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you say serde uses "crate?" Can we also do that for consistency?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

crate is a Rust keyword, is it okay if I use Crate instead?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry for the delay getting back to you. thanks for your patience. I would prefer crate if we can since that's what serde does. Is there a technical reason we can't do that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just overcome the technical difficulties, see the latest commit, now we have

#[derive(Debug, Eq, PartialEq, EnumIter)]
#[strum(crate = "nested::module::strum")]
enum Week {
    Sunday,
    Monday,
    Tuesday,
    Wednesday,
    Thursday,
    Friday,
    Saturday,
}

strum_macros/src/macros/enum_count.rs Outdated Show resolved Hide resolved
strum_macros/src/macros/enum_iter.rs Outdated Show resolved Hide resolved
@billy1624
Copy link
Contributor Author

Please review @Peternator7

@Peternator7
Copy link
Owner

lgtm! Thanks for the contribution. I'm on vacation next week, but I'll try and get a release out if I find a little time!

@Peternator7 Peternator7 merged commit e9d3b59 into Peternator7:master Jul 17, 2021
@billy1624
Copy link
Contributor Author

Have a nice vacation!!

@KeyboardDanni
Copy link

Hi, this would be very useful for my crate, as I'm in the process of refactoring my framework to improve ergonomics, and strum is currently used as an implementation detail. It would be nice to be able to put this into a macro and not require the user to manually pull in the strum crates.

Could a new release be made on crates.io so I can use this? Thanks!

@Peternator7
Copy link
Owner

Hi @cosmicchipsocket and @billy1624, thanks for the push. Version 0.22 is live on crates.io

@billy1624
Copy link
Contributor Author

Thanks! @cosmicchipsocket

@rbtcollins rbtcollins mentioned this pull request Oct 11, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Peternator7 Peternator7 Peternator7 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@billy1624 @Peternator7 @KeyboardDanni